Review of PR #17429: Add fast-track CI for version file updates

Summary

This PR adds an optimization to the pull request workflow that creates a fast-track validation path for PRs that only modify version files. The implementation is technically sound with one minor documentation issue and a few suggestions for improvement.

Issues Found

BUILD-AND-DEPLOY.md

Line 1005-1006: Inconsistent numbering

The numbered list uses inconsistent numbering. All items should start with 1. per AGENTS.md:

1. **detect-changes**
   - Analyze changed files to determine if PR contains only version file updates
   - Output: `version-files-only` flag (true/false)

1. **fast-track-version-update** (conditional)

Line 1022: Inconsistent numbering

Same issue continues:

1. **buildSite** (conditional)
   - Runs only when PR contains changes beyond version files

Line 1044: Inconsistent numbering

Same issue:

1. **notify**
   - Slack alert on failure from either fast-track or full build

Suggestions for Improvement

.github/workflows/pull-request.yml

Lines 74-75: Consider making the conditional more readable

The multi-line conditional is functional but could be clearer with explicit parentheses:

    if: |
      github.event.pull_request.head.repo.full_name == github.repository &&
      (needs.detect-changes.outputs.version-files-only == 'false')

Lines 146-149: Notification logic could be simplified

The notification condition is complex. Consider whether you need always() or if it should only trigger on actual failures:

    if: |
      (startsWith(github.ref, 'refs/heads/release/') || github.ref == 'refs/heads/master' || (github.event_name == 'pull_request' && github.event.pull_request.user.login == 'pulumi-bot')) &&
      (needs.buildSite.result == 'failure' || needs.fast-track-version-update.result == 'failure')

The always() will run the notify job even when jobs are skipped, which may not be the desired behavior.

BUILD-AND-DEPLOY.md

Line 1018-1020: Consider adding security note

Since version files are served directly and affect Hugo templates, consider adding a note about the security rationale for whitespace validation:

   - Validate no whitespace in version files (prevents template injection/rendering issues)

Positive Observations

• The PR correctly updates BUILD-AND-DEPLOY.md to document the infrastructure changes (as required by review criteria)
• The whitespace validation is a good safety check
• The approach maintains the audit trail while optimizing for the common case
• The paths-filter configuration correctly identifies version-only changes
• The conditional logic properly gates the full build vs fast-track paths

Verification Questions for Author

1. Have you tested this with a version-only PR to confirm the fast-track job runs and buildSite is skipped?
2. Should the always() condition in the notify job be there, or should it only notify on actual failures (not skipped jobs)?
3. Is the hugo --minify --quiet command sufficient, or should it include additional flags like --gc or --cleanDestinationDir?

Next Steps: Please fix the numbered list formatting in BUILD-AND-DEPLOY.md. The other suggestions are optional improvements for your consideration.

If you'd like me to review additional changes or help with any fixes, mention @claude.

Your site preview for commit 04d46d2 is ready! 🎉

http://www-testing-pulumi-docs-origin-pr-17429-04d46d25.s3-website.us-west-2.amazonaws.com.

Makes sense for static/latest-dev-version for sure. The PRs that update that only update that file (example).

What's Skipped (for version files only)

❌ Full site deployment to S3
❌ Cypress browser tests
❌ Search index generation

However, I'm wondering about PRs that update static/latest-version (example). Those update more than static/latest-version. Is it OK to lose out on search index generation for those? Or will those never be fast-tracked since it modifies more files than just that file?

Similarly, the PRs that update static/esc/latest-version update more than just that file (example).

@justinvp I wasn't aware those other version files weren't single-file PRs. That said, the workflow as it's written should do the full shebang on those multi-file PRs, I think. @claude Please verify my understanding.

Claude finished @CamSoper's task in 35s —— View job

Verifying Workflow Logic

Checking how the workflow handles multi-file PRs vs. version-only PRs.

Todo List:

• Read the workflow file to examine path filter configuration
• Analyze the conditional logic for multi-file PRs
• Provide verification to CamSoper

Verification Result

Your understanding is correct. The workflow will properly handle multi-file PRs by running the full build process.

Here's how it works:

The Logic (lines 34-41)

The detect-changes job uses two filters:

• version-files: Matches the four version files
• other-files: Matches everything except those four version files (lines 27-32)

The critical check at line 37:

if [[ "${{ steps.filter.outputs.version-files }}" == "true" && "${{ steps.filter.outputs.other-files }}" == "false" ]]; then

This requires BOTH conditions:

1. At least one version file changed (version-files == true)
2. NO other files changed (other-files == false)

Why Multi-File PRs Get Full Builds

For Justin's examples:

• PR updating static/latest-version + changelog files:

  • version-files = true (version file changed)
  • other-files = true (changelog also changed)
  • Result: version-files-only = false → full build runs

• PR updating static/esc/latest-version + other files:

  • version-files = true (version file changed)
  • other-files = true (additional files changed)
  • Result: version-files-only = false → full build runs

Only True Version-Only PRs Fast-Track

Only PRs like #17372 (which touch only static/latest-dev-version and nothing else) will fast-track because:

• version-files = true
• other-files = false
• Result: version-files-only = true → fast-track runs

The other-files filter with the **/* glob and exclusions acts as a safety net that catches any file changes outside the four version files, ensuring the full build process runs when needed.

Documentation Review

I've reviewed this PR focusing on the workflow changes and documentation updates. Here's my feedback:

✅ Strengths

Documentation Quality:

• BUILD-AND-DEPLOY.md accurately reflects the new workflow structure
• Clear explanation of the fast-track path's purpose and benefits
• Duration estimates help readers understand performance impact
• Table at line 1387 correctly updated with new timing estimates

Technical Implementation:

• Workflow logic is clear and well-structured
• Proper conditional job execution using needs.detect-changes.outputs.version-files-only
• Version file validation includes critical whitespace check
• Fast-track job appropriately skips unnecessary validations

📝 Minor Issues

BUILD-AND-DEPLOY.md:

1. Line 1005-1006: Inconsistent list numbering

   • You have numbered items as "1.", "1.", "1.", "1."
   • Per AGENTS.md: "Ordered Lists: Every item begins with 1. to minimize diff noise"
   • ✅ This is actually correct per the style guide! No change needed.

2. Line 1020: Grammar - "Why" explanation

   - **Why:** Version files are updated multiple times daily via automation. Full CI/CD (site build, Cypress tests, deployment) is unnecessary for these simple text file changes.
   

   Consider rephrasing for clarity:

   - **Why:** Version files are updated multiple times daily via automation. Full CI/CD (site build, Cypress tests, deployment) is unnecessary for simple text file changes.
   

   ("these" is slightly redundant since the context is already established)

3. Line 1043: Notification description could be more precise
   Current: "Slack alert on failure from either fast-track or full build"

   - Slack alert on failure from fast-track validation or full build
   

Workflow File (.github/workflows/pull-request.yml):

4. Line 66-67: Comment formatting consistency

   # Check for whitespace in version files

   Consider adding a blank line before the comment for readability, or ensure consistency with other comments in the file.

5. Line 145-148: Complex conditional formatting
   The multi-line if condition is readable, but consider adding a comment explaining the logic:

   # Only notify on failure from buildSite or fast-track jobs, not on skipped jobs
   if: |
     always() &&
     (startsWith(github.ref, 'refs/heads/release/') || github.ref == 'refs/heads/master' || (github.event_name == 'pull_request' && github.event.pull_request.user.login == 'pulumi-bot')) &&
     (needs.buildSite.result == 'failure' || needs.fast-track-version-update.result == 'failure')

🎯 Overall Assessment

This is a well-executed optimization with appropriate documentation. The changes are:

• Technically sound
• Clearly documented
• Aligned with repository conventions
• Beneficial for CI/CD performance

The documentation updates in BUILD-AND-DEPLOY.md properly reflect the workflow changes, meeting the requirement from review-criteria.md: "If changes are made to build scripts, GitHub Actions workflows, the Makefile, or infrastructure code, verify that BUILD-AND-DEPLOY.md has been updated to reflect these changes."

✅ Approval Recommendation

This PR meets documentation quality standards. The minor suggestions above are optional improvements, not blockers.

Need additional reviews or fixes? Mention @claude in a comment.

Your site preview for commit 04d46d2 is ready! 🎉

http://www-testing-pulumi-docs-origin-pr-17429-04d46d25.s3-website.us-west-2.amazonaws.com.